Configuration used: defaults

Review profile: CHILL

Plan: Pro

cortex/first_run_wizard.py (1)

390-390: Fix trailing whitespace on line 390.

Line 390 contains trailing whitespace, causing pipeline failures.

tests/test_ask.py (1)

253-253: Fix trailing whitespace on blank lines.

Multiple blank lines contain trailing whitespace (lines 253, 256, 261, 267), causing linting failures. These were flagged in the previous review and need to be addressed.

As per pipeline failures and coding guidelines.

Also applies to: 256-256, 261-261, 267-267

tests/test_ask.py (2)

252-252: Remove redundant import.

The os module is already imported at the top of the file (line 3), so this import statement is unnecessary.

🔎 Proposed fix

     def test_default_model_ollama(self):
         """Test default model for Ollama."""
         # Test with environment variable
-        import os

         # Save and clear any existing OLLAMA_MODEL
         original_model = os.environ.get("OLLAMA_MODEL")

As per PEP 8 coding guidelines.

---

268-272: Consider verifying the default fallback value.

The test correctly validates that a model is configured, but it doesn't verify the specific fallback default. According to the PR objectives, when neither OLLAMA_MODEL nor the config file specifies a model, it should default to "llama3.2".

Consider adding a test case that explicitly verifies this fallback behavior, possibly by temporarily moving/mocking the config file to ensure the default is tested in isolation.

💡 Example approach

You could add an additional test or expand this one to verify the default:

# Test with mocked config that returns None for ollama_model
with patch('cortex.ask.load_config', return_value={}):
    handler3 = AskHandler(api_key="test", provider="ollama")
    self.assertEqual(handler3.model, "llama3.2")

This would ensure the fallback default is properly tested per the PR objectives.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

cortex/first_run_wizard.py (1)

389-394: Critical: KeyError on invalid model choice.

Line 394 will raise KeyError if the user enters an invalid choice (e.g., "6", "abc", or any value not in "1"-"5"). The code only checks for "5" at line 391, but doesn't validate "1"-"4" before the dictionary lookup.

🔎 Proposed fix

         choice = self._prompt("\nEnter choice [1]: ", default="1")

         if choice == "5":
             model_name = self._prompt("Enter model name: ", default="llama3.2")
         else:
-            model_name = model_choices[choice]
+            model_name = model_choices.get(choice, "llama3.2")

Alternatively, add input validation:

         choice = self._prompt("\nEnter choice [1]: ", default="1")
+        
+        # Validate choice
+        while choice not in model_choices and choice != "5":
+            print("Invalid choice. Please enter a number between 1 and 5.")
+            choice = self._prompt("\nEnter choice [1]: ", default="1")

         if choice == "5":
             model_name = self._prompt("Enter model name: ", default="llama3.2")
         else:
             model_name = model_choices[choice]

tests/test_ask.py (1)

245-257: Consider using @patch.dict for cleaner test isolation.

The test manually saves and restores the OLLAMA_MODEL environment variable. While this works, using @patch.dict(os.environ, ...) provides automatic cleanup even if the test fails and is more consistent with other tests in the file (e.g., test_install_no_api_key in test_cli.py).

🔎 Proposed refactor

-    def test_default_model_ollama(self):
+    @patch.dict(os.environ, {"OLLAMA_MODEL": "test-model"})
+    def test_default_model_ollama(self):
         """Test default model for Ollama."""
-        # Test with environment variable
-
-        # Save and clear any existing OLLAMA_MODEL
-        original_model = os.environ.get("OLLAMA_MODEL")
-
         # Test with custom env variable
-        os.environ["OLLAMA_MODEL"] = "test-model"
         handler = AskHandler(api_key="test", provider="ollama")
         self.assertEqual(handler.model, "test-model")
-
-        # Clean up
-        if original_model is not None:
-            os.environ["OLLAMA_MODEL"] = original_model
-        else:
-            os.environ.pop("OLLAMA_MODEL", None)

Note: You'll need to test the default behavior separately, potentially in a new test method without the env var set.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

tests/test_cli.py (1)

62-71: Test correctly validates Ollama fallback behavior.

The test properly mocks CommandInterpreter and verifies that installation succeeds with Ollama when no API key is available. The mocking pattern is consistent with other similar tests in this file.

💡 Optional: Consider adding assertions to verify provider selection

To make the test more explicit about Ollama fallback behavior, you could add assertions to verify the provider and API key:

@patch.dict(os.environ, {}, clear=True)
@patch("cortex.cli.CommandInterpreter")
def test_install_no_api_key(self, mock_interpreter_class):
    # Should work with Ollama (no API key needed)
    mock_interpreter = Mock()
    mock_interpreter.parse.return_value = ["apt update", "apt install docker"]
    mock_interpreter_class.return_value = mock_interpreter
    
    result = self.cli.install("docker")
    # Should succeed with Ollama as fallback provider
    self.assertEqual(result, 0)
    
    # Verify Ollama was used as the provider
    mock_interpreter_class.assert_called_once()
    call_kwargs = mock_interpreter_class.call_args[1]
    self.assertEqual(call_kwargs.get("provider"), "ollama")
    self.assertEqual(call_kwargs.get("api_key"), "ollama-local")

This would make the Ollama fallback behavior more explicit in the test verification.

Configuration used: defaults

Review profile: CHILL

Plan: Pro